Add update-signatures command to migrate deprecated signatures #368

jhall1-godaddy · 2022-07-17T21:12:11Z

To help us get this pull request reviewed and merged quickly, please be sure to include the following items:

Tests (if applicable)
Documentation (if applicable)
Changelog entry
A full explanation here in the PR description of the work done

PR Type

What kind of change does this PR introduce?

Backward Compatibility

Is this change backward compatible with the most recently released version? Does it introduce changes which might change the user experience in any way? Does it alter the API in any way?

Yes (backward compatible)
No (breaking changes)

What's new?

This pull request addresses the recent deprecation of some signatures which were generated with the leading +/- from the git diff. That issue is documented here.

As stated by the deprecation warning, in tartufo v4.X those signatures will no longer be valid.
In an attempt to ease the migration for all parties involved, this command allows for migration with a single command.

The command performs a local scan (we assume the user is scanning locally, this needs to be documented), parses the deprecations from the ouput there, and makes adjustments in the end users tartufo config file.

Help snippet:

Commands:
  scan-local-repo    Scan a repository already cloned to your local system.
  scan-folder        Scan a folder.
  update-signatures  Update deprecated signatures for a local repository.
  pre-commit         Scan staged changes in a pre-commit hook.
  scan-remote-repo   Automatically clone and scan a remote git repository.

Invoked identically to the scan-local-repo command, it accepts all the same options as well.

E.G. tartufo update-signatures .

Usage

I used tartufo itself for testing, and will include my results here.
Prior to updating the signatures tartufo was producing the following deprecations:

/home/jonx/projects/work/tartufo/tartufo/scanner.py:661: DeprecationWarning: Signature 358a1ae040ab71b3aafeb4318e5d9e318ff1b681c5bf0e99d381584a33ee7833 was generated by an old version of tartufo and is deprecated. tartufo 4.x will not recognize this signature. Please update your configuration to use signature 38d3398905c382a79857e884f33062a5a38e4e3f8b5d54f269879601b2e847fc instead.
  warnings.warn(
/home/jonx/projects/work/tartufo/tartufo/scanner.py:661: DeprecationWarning: Signature 358a1ae040ab71b3aafeb4318e5d9e318ff1b681c5bf0e99d381584a33ee7833 was generated by an old version of tartufo and is deprecated. tartufo 4.x will not recognize this signature. Please update your configuration to use signature 38d3398905c382a79857e884f33062a5a38e4e3f8b5d54f269879601b2e847fc instead.
  warnings.warn(
/home/jonx/projects/work/tartufo/tartufo/scanner.py:661: DeprecationWarning: Signature 6c70b4a712ada69f2a6c919edc783cf485b6637e3c5429de376ef1a451a042e4 was generated by an old version of tartufo and is deprecated. tartufo 4.x will not recognize this signature. Please update your configuration to use signature d26b223efb2087d4a3db7b3ff0f65362372c46190fd74b8061db8099f4a2ee60 instead.
  warnings.warn(
/home/jonx/projects/work/tartufo/tartufo/scanner.py:661: DeprecationWarning: Signature 357841766ca69979608e4e7fb1da2addeafe6d15cade15466c39d903cc0488a8 was generated by an old version of tartufo and is deprecated. tartufo 4.x will not recognize this signature. Please update your configuration to use signature 94c45f9acae0551a2438fa8932fac466e903fd9a09bd2e11897198b133937ccd instead.
  warnings.warn(
/home/jonx/projects/work/tartufo/tartufo/scanner.py:661: DeprecationWarning: Signature 88152f1d10077f417ee5aea5fa86adaa5b74d3f2de2e87ce4b4aa5cd79c7b2f5 was generated by an old version of tartufo and is deprecated. tartufo 4.x will not recognize this signature. Please update your configuration to use signature 482e7b31188f1e59dd6eb5b21c46f7467449ffcbeba9a4ca412dfcd8ea9ef66f instead.
  warnings.warn(
/home/jonx/projects/work/tartufo/tartufo/scanner.py:661: DeprecationWarning: Signature 23996f9309c042d37958d28c2cc0c16a060a03376ee075b5b077609e2a299e44 was generated by an old version of tartufo and is deprecated. tartufo 4.x will not recognize this signature. Please update your configuration to use signature 482e7b31188f1e59dd6eb5b21c46f7467449ffcbeba9a4ca412dfcd8ea9ef66f instead.
  warnings.warn(
/home/jonx/projects/work/tartufo/tartufo/scanner.py:661: DeprecationWarning: Signature 85109edb8081a60f3a44139a98b643b8cacb777f4d5c814276ae729d50fe414c was generated by an old version of tartufo and is deprecated. tartufo 4.x will not recognize this signature. Please update your configuration to use signature bf48e316ecad0a37071ed81cc54a6b629a1a3bf73cf52d06a77c9fbaf91bc833 instead.
  warnings.warn(
Time: 2022-07-17T13:36:54.860205
All clear. No secrets detected.

Running the command:

$ tartufo update-signatures .                                                                                                                                                                            
Found 6 unique deprecated signatures.
1) Updating '23996f9309c042d37958d28c2cc0c16a060a03376ee075b5b077609e2a299e44' -> '482e7b31188f1e59dd6eb5b21c46f7467449ffcbeba9a4ca412dfcd8ea9ef66f'
2) Updating '6c70b4a712ada69f2a6c919edc783cf485b6637e3c5429de376ef1a451a042e4' -> 'd26b223efb2087d4a3db7b3ff0f65362372c46190fd74b8061db8099f4a2ee60'
3) Updating '88152f1d10077f417ee5aea5fa86adaa5b74d3f2de2e87ce4b4aa5cd79c7b2f5' -> '482e7b31188f1e59dd6eb5b21c46f7467449ffcbeba9a4ca412dfcd8ea9ef66f'
4) Updating '358a1ae040ab71b3aafeb4318e5d9e318ff1b681c5bf0e99d381584a33ee7833' -> '38d3398905c382a79857e884f33062a5a38e4e3f8b5d54f269879601b2e847fc'
5) Updating '357841766ca69979608e4e7fb1da2addeafe6d15cade15466c39d903cc0488a8' -> '94c45f9acae0551a2438fa8932fac466e903fd9a09bd2e11897198b133937ccd'
6) Updating '85109edb8081a60f3a44139a98b643b8cacb777f4d5c814276ae729d50fe414c' -> 'bf48e316ecad0a37071ed81cc54a6b629a1a3bf73cf52d06a77c9fbaf91bc833'
Updated 6 total deprecated signatures.

After running the command, the pyproject.toml was altered as follows:

diff --git a/pyproject.toml b/pyproject.toml
index 8088fce..0e50d97 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -96,8 +96,8 @@ exclude-signatures = [
   {signature = "dbc084c22395931afd734a5be53f1cde913c7d988034b856087c744b424d9673", reason = "High entropy string BASE64_CHARS in truffleHog/truffleHog.py 9-Jan-2017"},
   {signature = "bc67523b08b2170ca9b802535684454140679af3156bff808508cf32b33aa240", reason = "High entropy string random_stringHex in tests.py 28-Sep-21017"},
   {signature = "978dc8605ef9bf471e467dd2d8570fd658f0a5f25378799a2ad8bb6ea480173d", reason = "High entropy string random_stringB64 in tests.py 28-Sep-21017"},
-  {signature = "85109edb8081a60f3a44139a98b643b8cacb777f4d5c814276ae729d50fe414c", reason = "High entropy string in secretFile.txt 30-Dec-2016"},
-  {signature = "23996f9309c042d37958d28c2cc0c16a060a03376ee075b5b077609e2a299e44", reason = "High entropy string in temp/nothing 30-Dec-2016"},
+  {signature = "bf48e316ecad0a37071ed81cc54a6b629a1a3bf73cf52d06a77c9fbaf91bc833", reason = "High entropy string in secretFile.txt 30-Dec-2016"},
+  {signature = "482e7b31188f1e59dd6eb5b21c46f7467449ffcbeba9a4ca412dfcd8ea9ef66f", reason = "High entropy string in temp/nothing 30-Dec-2016"},
   {signature = "51dad1be0950e627bb3660a6246b42ddc6d6d431267b309af2469114fd2df2e5", reason = "Regular expression match, RSA private key in truffleHog/regexChecks.py 8-Dec-2017"},
   {signature = "74c056ae650ffb202ea3b68207dce82ebeae0414cfe3b49f9db0c79841473da1", reason = "Regular expression match, PGP private key block in truffleHog/regexChecks.py 8-Dec-2017"},
   {signature = "36617c71d89b78193c99fdd2a28027319dfdb361d649372bc72c45317f9edf24", reason = "Regular expression match, SSH (EC) private key in truffleHog/regexChecks.py 8-Dec-2017"},
@@ -131,13 +131,13 @@ exclude-signatures = [
   {signature = "586f9df1870fcfafd3070dc553e97db25b3fce82c9144602567d69a86597c06f", reason = "High entropy string commit_w_secret in tests/test_scanner.py 11-Nov-2019"},
   {signature = "842533f44cf32fb3d93a7f56227977aa4f16caafe82ace8f5f4de27d750c1ec1", reason = "High entropy string since_commit in tests/test_scanner.py 11-Nov-2019"},
   {signature = "d039c652f27c4d42026c5b5c9be31bfe368b283b24248e98d92f131272580053", reason = "High entropy string BASE64_CHARS in tartufo/scanner.py 25-Aug-2020"},
-  {signature = "6c70b4a712ada69f2a6c919edc783cf485b6637e3c5429de376ef1a451a042e4", reason = "tests/data/scan_folder/donotscan.txt"},
-  {signature = "357841766ca69979608e4e7fb1da2addeafe6d15cade15466c39d903cc0488a8", reason = "tests/data/scan_folder/test.txt"},
+  {signature = "d26b223efb2087d4a3db7b3ff0f65362372c46190fd74b8061db8099f4a2ee60", reason = "tests/data/scan_folder/donotscan.txt"},
+  {signature = "94c45f9acae0551a2438fa8932fac466e903fd9a09bd2e11897198b133937ccd", reason = "tests/data/scan_folder/test.txt"},
   {signature = "743b893e30777d9c4e28d3d341ca4ba1c2541a9c62b497dece75c2a64420e770", reason = "tests/test_folder_scanner.py"},
-  {signature = "358a1ae040ab71b3aafeb4318e5d9e318ff1b681c5bf0e99d381584a33ee7833", reason = "tests/data/scan_folder/scan_sub_folder/sub_folder_test.txt"},
+  {signature = "38d3398905c382a79857e884f33062a5a38e4e3f8b5d54f269879601b2e847fc", reason = "tests/data/scan_folder/scan_sub_folder/sub_folder_test.txt"},
   {signature = "9d1b278fa384c9dc58d607130d97740910309d13b29f13f7ed6348738ea4f32c", reason = "tests/test_folder_scanner.py 16-Nov-2021"},
   {signature = "9f675d4d7f43e484e07fb199a1e7ec372ef204631c1f4ec054acc3119fbde4cf", reason = "build_and_publish_docker_file 13-Jun-2019"},
-  {signature = "88152f1d10077f417ee5aea5fa86adaa5b74d3f2de2e87ce4b4aa5cd79c7b2f5", reason = "nothing file, 30-Dec-2016"},
+  {signature = "482e7b31188f1e59dd6eb5b21c46f7467449ffcbeba9a4ca412dfcd8ea9ef66f", reason = "nothing file, 30-Dec-2016"},
   {signature = "c94ba36a7411bf89fab6e87076740deecdc7a61fcfbbc1aa5934608f6c4f3adb", reason = "tests/data/config/rule_pattern_config.toml"},
   {signature = "4f44b817b42c94fa01f47166ca7adaa0b750b1b6cd84f7ce3bf23a7728cdac57", reason = "commit hashes in github URLs in comments, tartufo/scanner.py"}
 ]

Thoughts

Tests should be feature complete, with full coverage.
I took a more functional approach here because frankly this command didn't feel like it would benefit from OOP in any way.
I haven't explicitly written any documentation other than the stuff added by click and sphinx automatically from docstrings, I am interested in getting feedback on how that should be structured.

Only print update message if sigs were altered

Refactor typing imports and function names

rbailey-godaddy

This looks good to me, but I have an additional challenge based on your sample output. :)

The old algorithm could actually generate three different signatures for the exact same secret under these circumstances, when it appeared at the beginning of a line:

When introducing/modifying the secret (where it could have been prefixed with + in error)
When removing/modifying the secret (where it could have been prefixed with - in error)
When the secret was adjacent to an unrelated change (and it could appear as part of the diff context, where it would be prefixed with a space)

Note in the last case, the space would not be considered part of the secret (because it is not in the base64 encoding alphabet) and the "old" signature actually is identical to the "new" signature.

Given this behavior, it's probably common that multiple old signatures may collapse to the same new signature (as demonstrated in your example). While this is totally acceptable, I can't help wondering if it is possible to detect the duplication and simply delete the original signature instead of replacing it with a duplicate new signature.

Regardless of technical merit, I also could see a case made for leaving the duplicates anyway in order to reduce confusion when modifying non-trivial configurations.

tartufo/commands/update_signatures.py

rbailey-godaddy · 2022-07-18T14:12:49Z

One additional thought on this very nice piece of work. Could you attach a flag to update-signatures such as --replace-deprecated-signatures which triggers this behavior? (I am not in love with this option name and if you think of something you like better, use what you like.) I am trying to make room for additional future behavior such as --remove-unused or --replace-deprecated-format that would fit naturally into this verb.

For convenience, we could have --replace-deprecated-signatures be the default and let people specify --no-replace-deprecated-signatures when they want that.

Thanks again for a very timely contribution!

rbailey-godaddy · 2022-07-18T14:13:33Z

Maybe update-configuration would be better than update-signatures with respect to my last comment?

jhall1-godaddy · 2022-07-18T14:21:47Z

I can't help wondering if it is possible to detect the duplication and simply delete the original signature instead of replacing it with a duplicate new signature.

This is an interesting thought. At the very least we could warn of any duplicates and let the user handle it. Removing them would be pretty trivial - but which one is the correct one to remove? Maybe best to let them decide.

jhall1-godaddy · 2022-07-18T14:25:21Z

update-configuration feels fitting. Will get that done.

sushantmimani

I am happy with these changes once @rbailey-godaddy's comments have been addressed. In terms of documentation, I feel that addition of a new paragraph in upgrading3.rst explaining the new features and how to use it should be enough.

--update-configuration (default: True) --remove-duplicates (default: True) Fix return type on util.fail function Remove redundant assert Add tests

Make the output messages more similar

jhall1-godaddy · 2022-07-19T20:25:45Z

Changed the return type of util.fail to typing.NoReturn which allowed me to remove the assert.

Added 2 flag options to the command:
--update-configuration and --no-update-configuration - Default is to update the config.
--remove-duplicates and --no-remove-duplicates - Default is to remove duplicates.

In the case of removing duplicates, the first instance of the signature is kept and the rest are discarded.
Added a paragraph to upgrading3.rst regarding this command and its use case.

rbailey-godaddy

This is really great work. Many thanks for the contribution, which makes life better for both users and developers!

sushantmimani

Good stuff!

jhall1-godaddy added 7 commits July 14, 2022 15:21

Initial impl of signature migration subcommand

f0df739

Lint and format

c3b3d5d

Use an external counter in case sigs are duplicated

bc9a397

Only print update message if sigs were altered

Refactor logic into functions

5ecec2a

Only write the config file if changes were made

7529c0d

Refactor typing imports and function names

Add test suite for update signatures command

7b87c50

Remove unnecessary file read

c395268

jhall1-godaddy requested a review from a team as a code owner July 17, 2022 21:12

jhall1-godaddy requested review from sushantmimani, rfox-godaddy and sjacoby-godaddy July 17, 2022 21:12

jhall1-godaddy added 4 commits July 17, 2022 14:14

Add changelog fragment

8209467

Add missing comma

14e7d09

Remove extra newline in tests

c5d056b

Docstring fix

05f7436

rbailey-godaddy reviewed Jul 18, 2022

View reviewed changes

tartufo/commands/update_signatures.py Outdated Show resolved Hide resolved

tartufo/commands/update_signatures.py Show resolved Hide resolved

tartufo/commands/update_signatures.py Outdated Show resolved Hide resolved

sushantmimani reviewed Jul 18, 2022

View reviewed changes

jhall1-godaddy added 6 commits July 19, 2022 12:06

Add 2 flags to update-signatures:

0c13d08

--update-configuration (default: True) --remove-duplicates (default: True) Fix return type on util.fail function Remove redundant assert Add tests

Add documentation to upgrading3 about updating signatures

f4697c3

Account for ordering mismatch in older version of python

ca7d9a4

Add plurality to found deprecated signature output

da598d4

Make the output messages more similar

Add note about keeping first duplicate and discarding the rest

bbeadf6

Remove comma

d1e878d

Formatting

8f5c5dd

rbailey-godaddy approved these changes Jul 20, 2022

View reviewed changes

sushantmimani approved these changes Jul 20, 2022

View reviewed changes

sushantmimani merged commit 0640033 into godaddy:main Jul 20, 2022

jhall1-godaddy deleted the signature-migration branch July 21, 2022 13:07

sushantmimani mentioned this pull request Feb 28, 2023

Command to Upgrade Config to 3.x #313

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add update-signatures command to migrate deprecated signatures #368

Add update-signatures command to migrate deprecated signatures #368

jhall1-godaddy commented Jul 17, 2022 •

edited

Loading

rbailey-godaddy left a comment

rbailey-godaddy commented Jul 18, 2022

rbailey-godaddy commented Jul 18, 2022

jhall1-godaddy commented Jul 18, 2022

jhall1-godaddy commented Jul 18, 2022

sushantmimani left a comment

jhall1-godaddy commented Jul 19, 2022

rbailey-godaddy left a comment

sushantmimani left a comment

Add update-signatures command to migrate deprecated signatures #368

Add update-signatures command to migrate deprecated signatures #368

Conversation

jhall1-godaddy commented Jul 17, 2022 • edited Loading

PR Type

Backward Compatibility

What's new?

Usage

Thoughts

rbailey-godaddy left a comment

Choose a reason for hiding this comment

rbailey-godaddy commented Jul 18, 2022

rbailey-godaddy commented Jul 18, 2022

jhall1-godaddy commented Jul 18, 2022

jhall1-godaddy commented Jul 18, 2022

sushantmimani left a comment

Choose a reason for hiding this comment

jhall1-godaddy commented Jul 19, 2022

rbailey-godaddy left a comment

Choose a reason for hiding this comment

sushantmimani left a comment

Choose a reason for hiding this comment

jhall1-godaddy commented Jul 17, 2022 •

edited

Loading